chore: Support dml_batch_update_count option in PG dialect - #4142
Conversation
product-auto-label
Bot
added
size: m
sakthivelmanii
changed the title
5f1b158 to
6a5b11f
Compare
| "auto_batch_dml_update_count"; | ||
| public static final String AUTO_BATCH_DML_UPDATE_COUNT_VERIFICATION_PROPERTY_NAME = | ||
| "auto_batch_dml_update_count_verification"; | ||
| public static final String DML_BATCH_UPDATE_COUNT_PROPERTY_NAME = "dml_batch_update_count"; |
There was a problem hiding this comment.
I think that the name batch_dml_update_count would be better, as it more in line with auto_batch_dml_update_count.
There was a problem hiding this comment.
@olavloite I initially named it that way. I changed it by thinking that it would be breaking change since PGAdapter supports this way.
There was a problem hiding this comment.
Yeah, that is a good point. PGAdapter will automatically use the implementation in the Connection API in a case like that. If the behavior is exactly the same, then that should then not be a problem. But it might be good to verify that by running the PGAdapter tests locally with version of the client library.
| "exampleStatements": [ | ||
| "set local spanner.dml_batch_update_count = 0", | ||
| "set local spanner.dml_batch_update_count = 100", | ||
| "set local spanner.dml_batch_update_count to 1" |
There was a problem hiding this comment.
nit: add an example statement that does not use local. The test generator uses these example statements to generate tests, so adding an example without local also means that you get tests for this statement without the local keyword.
There was a problem hiding this comment.
I am wondering do we need to support session itself? is local not enough?
| public void testDmlBatchUpdateCount() { | ||
| SpannerPool.closeSpannerPool(); | ||
| mockSpanner.putStatementResult( | ||
| MockSpannerServiceImpl.StatementResult.detectDialectResult(Dialect.POSTGRESQL)); |
There was a problem hiding this comment.
This test should test both dialects
There was a problem hiding this comment.
No. It didn't run for both. I have made changes to run it for both.
| "converterName": "ClientSideStatementValueConverters$LongConverter" | ||
| } | ||
| }, | ||
| { |
There was a problem hiding this comment.
Run the following command in the google-cloud-spanner folder:
mvn -Ddo_log_statements=true exec:java -Dexec.mainClass=com.google.cloud.spanner.connection.SqlTestScriptsGenerator -Dexec.classpathScope="test"
This will generate tests for the new statements that you have added to the client-side statement file(s).
See ConnectionImplGeneratedSqlScriptTest for more information.
e436f29 to
1000fdc
Compare
product-auto-label
Bot
added
size: xl
| base_branch: main | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
nit: undo unrelated change? (Maybe in a follow-up PR)
There was a problem hiding this comment.
it's automatically added by bot.
|
@olavloite addressed the comments. |
3 participants
Description:
Support setting batch DML update count for Postgres and GoogleSQL Dialect. When customer execute
set local spanner.dml_batch_update_count to 1(Postgres) ORset local dml_batch_update_count = 1(GoogleSQL) this will ensure updateCount for batch operations will always return 1(not auto batch dml).